-
Notifications
You must be signed in to change notification settings - Fork 302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: avoid importing net/http on wasm #449
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I understand the reason for this change but unfortunately I cannot merge it as it breaks the API. The API must be kept 1:1 between Go and Wasm and so Dial must return http.Response.
Dial is a global function and the value returned on wasm is a dummy value. Are people using these in interfaces? is that why? Other than that it seems like anyone using Dial would already be ignoring the dummy value on wasm. |
People could be wrapping this library in some kind of API/helper for their own code and they might be checking resp or passing it up the chain. This patch would break that. I don't know if it's common but it doesn't seem far fetched to me. I don't have a better suggestion unfortunately. I'll leave this open for now, let's see if anyone else has thoughts. |
Using goweight: https://github.com/paralin/goweight GOOS=js GOARCH=wasm goweight | less Without this change: 7.9 MB runtime 6.6 MB net/http 3.1 MB net 3.0 MB crypto/tls 2.0 MB reflect 1.4 MB math/big 1.3 MB crypto/x509 935 kB os ... With this change: 7.9 MB runtime 3.1 MB net 2.0 MB reflect 935 kB os ... Slight modifications to avoid importing net/http reduce the binary size significantly. Signed-off-by: Christian Stewart <[email protected]>
2358de6
to
db89d1b
Compare
Hey @paralin. Thanks for this proposal, the weight reduction is impressive but I believe this is an uphill battle. For instance, in #373 there's a request to introduce We also want to avoid breaking the API while we're v1, targeting API breakages for a potential v2 in the future is of course an option, but will not be done lightly. With these constraints in mind, I don't see how we could support this. The only option I can think of is to let users opt-in via a build tag, say But before we consider adding more complexity to the code-base, I'd like to hear about use-cases where this weight reduction is necessary. |
In a web browser app using wasm with Go, any weight reduction is significant. While going through the list of packages taking up space in the binary, net/http was at the top of the list. 6.6 MB net/http
That's around 1 second of time spent downloading net/http, and even a second delay loading a website feels like a lot. Of course gzip / brotli compression reduces this significantly. I understand the desire to keep API compatibility. I also use fetch on wasm to reduce binary size: https://github.com/aperturerobotics/util/blob/master/js/fetch/fetch.go |
@paralin what you say makes sense but what I'm looking for is concrete use-cases. Code where you're using If I'd write a program only using
Sure, |
The concrete use case is compiling a wasm binary and running it in a webpage, then using websocket.Dial. It's quite easy to make a program that dials a websocket and exchanges messages with a server in wasm that doesn't otherwise use net/http. That said if it requires a separate go tag I probably wouldn't use the feature and would just keep using my forked version. No worries if you don't feel that the 0.7MB difference is worth it. It definitely is in wasm apps though, and as wasm and go become more developed and widely used, this kind of size savings will become more valuable. At the moment it probably doesn't matter to anyone that wouldn't just fork the package for their use case anyway. |
Using goweight: https://github.com/paralin/goweight
GOOS=js GOARCH=wasm goweight | less
Without this change:
7.9 MB runtime
6.6 MB net/http
3.1 MB net
3.0 MB crypto/tls
2.0 MB reflect
1.4 MB math/big
1.3 MB crypto/x509
935 kB os
...
With this change:
7.9 MB runtime
3.1 MB net
2.0 MB reflect
935 kB os
...
Slight modifications to avoid importing net/http reduce the binary size significantly.